-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(NODE-6878): ensure documents are cleared correctly to handle emptyGe… #4488
base: main
Are you sure you want to change the base?
Conversation
Hey @italojs , Thanks for the submission and good catch! We can reproduce this issue as well. The culprit behind this issue seems to be a Typescript cast
We think that a more future-proof approach might be to directly instantiate a CursorResponse: static get emptyGetMore(): CursorResponse {
return new CursorResponse(
serialize({ ok: 1, cursor: { id: 0, nextBatch: [] } })
)
} Would you be willing to make this change? The static getter is important here to avoid serialization at the module-level and only serialize when needed. Also, we'll need a test for this scenario to confirm that the behavior has been fixed. Would you be willing to add a regression test? I believe
Without the changes in this PR, the above throws an error. I'm also happy to make these changes if you'd prefer, just let me know. Thanks again! |
f145592
to
1e8494e
Compare
- Refactor CursorResponse to use a getter for emptyGetMore, returning a serialized response. - Update AbstractCursor to safely handle the clear method on documents, ensuring proper reinitialization when necessary. - Add integration tests to verify cursor rewind functionality after applying the emptyGetMore optimization, addressing NODE-6878.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@italojs Seems like you're failing these tests
@W-A-James, How can I see the CI output tests? It's passing on my machine, but I would like to know if it's an environmental issue or a flaky error.
In the GitHub CI, it says I must have to a mongodb.com account. I'm a little bit lot on it to be honest ![]() |
Jira issue: https://jira.mongodb.org/browse/NODE-6878
MongoDB Driver:
documents?.clear() is not a function
error in AbstractCursor.rewind()Issue Description
When using the MongoDB Node.js driver, there's an issue in the
rewind()
method ofAbstractCursor
. The method attempts to callthis.documents?.clear()
without checking if theclear()
method exists on thedocuments
object.In certain scenarios, particularly when the MongoDB driver uses an optimization called
emptyGetMore
to avoid unnecessary server calls,this.documents
is set to a simplified object literal that lacks theclear()
method:This causes an error when
rewind()
is called on cursors that have this optimization applied:Impact
This issue can cause applications to crash when:
rewind()
emptyGetMore
Reproduction Details
In this repository, the
reproduction.js
script demonstrates this issue by:Running the Reproduction
Expected output will show the error:
Proposed Solution
The issue can be fixed by modifying the
rewind()
method to safely handle cases wheredocuments
doesn't have aclear()
method. Here's the proposed fix:Why This Solution Works
clear()
exists before calling itdocuments
tonull
achieves the same goal asclear()
- it resets the cursor's document bufferemptyGetMore
cursorsReferences